Repository navigation
Conversation
|
There is a performance concern with the unconditional per-block cache traversal in I compared the actual operator and changed transformer classes from the merge base
All three scenarios were slower in both forks. Method: JMH 1.37, Zulu JDK 17.0.4.1, one benchmark thread, average-time mode, 3 × 1 s warmup and 4 × 1 s measurement per fork, two forks per revision/scenario with revision order reversed in the second round, a 512 MiB G1 heap, and the GC allocation profiler. The transformer graphs were constructed directly and consumed eight rotating, prebuilt input blocks; setup checked every output against expected values before timing, and each measured invocation returned its result to JMH. These are operator microbenchmark results, not end-to-end SQL regression percentages. They do not include SQL planning, storage scanning, aggregation, or network costs. The 1024-row variants and the separate 16-independent-additions scenario had substantial variation between forks; a longer-warmup check also showed variation, so I am not drawing a stable, general query-latency regression percentage from those results. Suggested optimization: once the transformer DAG is fully constructed, collect and identity-deduplicate the nodes once, and retain a flat list for each evaluation phase. The filter-phase list should include the filter and common-expression roots; the projection-phase list should include all projection roots. Both should include CASE descendants. At each new-input boundary, iterate the corresponding list and clear only each node's own cache before initializing the leaves. Keep the existing evaluation-time CASE cleanup semantics unchanged; calling the recursive legacy This should preserve the cross-block correctness fix while removing repeated DAG traversal, identity-set allocation/resizing, and hash lookups from the per-block path. The same approach can be applied to the schema-batch handlers. I recommend addressing this overhead, particularly for workloads producing many small TsBlocks. |
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on the per-block traversal. The precomputed flat lists in 5e0cb1c address my earlier comment, and the original cross-block repro from #18792 now returns the correct rows.
Requesting changes for two problems in this PR:
- Regression in the schema UPDATE path: an UPDATE whose CASE guards a common expression now fails where base succeeds (inline on
DeviceAttributeUpdater). IoTDBCaseDeviceCacheIT#testAttributeCommonExpressionClearedByCasedoes not exercise the path it targets: it passes on base (inline).
Same mechanism, not fixed by this PR. Please fix them here or open follow-ups (details inline):
- Within a single block, CASE clears columns that a sibling operand has already computed but not read yet. Results: silent NULLs, NPEs, and
UPDATE … SET a = concat(src, CASE …)overwriting attributes with NULL. MultiColumnTransformer.evaluateWithSelectionreads the position count aftergetColumn(), soCASE … THEN coalesce(x, …) ENDreturns NULL.diff()under a CASE filter is evaluated twice per block. With this PR the multi-block case returns wrong values instead of failing.
Root cause / design suggestion: short-circuit (evaluateWithSelection) results are stored in the shared, ref-counted ColumnCache, so CASE has to clear caches during evaluation. That clearing destroys columns other consumers still need and forces recomputation within the block (3 and 5; combined with the unmasked recompute, also 1). Resetting at block boundaries cannot help inside a block. Keeping selection-mode results out of the shared cache (for example, returning them directly to the CASE), evaluating the first WHEN with tryEvaluate(), and dropping the evaluation-time clearCache() calls would remove that recompute path and leave the block-boundary reset as a safety net. Item 1 would still need the filter mask.
Out of scope (pre-existing, file not touched by this PR): tree-model LogicAndColumnTransformer.evaluateWithSelection caches rightColumn instead of builder.build() (L80). As a result, CASE WHEN s1 > 100 THEN 1 WHEN s1 > 0 AND s2 > 0 THEN 2 ELSE 3 END returns 2 instead of 3 when s1 is NULL and s2 = 1 (same on base). Probably worth a separate issue.
All SQL in this review was run on 1C1D instances built from this PR (50c38c1, key cases re-checked on 5e0cb1c) and from the merge base fbd5fd2.
| // CASE evaluation can clear a common expression after the filter used it. Recompute it before | ||
| // materializing the column for projection within this same batch. | ||
| commonTransformerList.forEach( | ||
| columnTransformer -> filterResultColumns.add(columnTransformer.getColumn())); | ||
| columnTransformer -> { | ||
| columnTransformer.tryEvaluate(); | ||
| filterResultColumns.add(columnTransformer.getColumn()); | ||
| }); |
There was a problem hiding this comment.
Regression (must fix): tryEvaluate() recomputes a cleared common expression for every device in the batch, ignoring the filter. If the CASE was guarding that expression, the UPDATE now evaluates it on devices the guard excluded:
CREATE TABLE t (id STRING TAG, src STRING ATTRIBUTE, a STRING ATTRIBUTE, v DOUBLE FIELD);
INSERT INTO t(time, id, src, a, v) VALUES (1, 'd0', 'n/a', 'seed', 1), (2, 'd1', '5', 'seed', 1);
UPDATE t SET a = CAST(CAST(src AS INT32) + 1 AS STRING)
WHERE CASE WHEN src <> 'n/a' THEN CAST(src AS INT32) > 10 ELSE false END;- base: succeeds; no device matches, so
astaysseed - this PR:
305: internal error. statemachine throws a runtime exception: org.apache.iotdb.commons.exception.SemanticException: Cannot cast n/a to INT32 type
If d1 has src = '50' instead (one device matches), base fails with the NPE this PR targets and this PR fails with the same cast error. The expected result is a = '51' for d1.
The query path has always behaved this way (FilterAndProjectOperator L191-L199): SELECT CAST(s AS INT32) + 1 FROM z4 WHERE CASE WHEN s <> 'n/a' THEN CAST(s AS INT32) > 10 ELSE false END fails with 701: Cannot cast n/a to INT32 type on both base and this PR.
Suggested fix for both call sites: recompute only the rows that passed the filter, for example
// ColumnTransformer
public void tryEvaluateWithSelection(boolean[] selection) {
if (!columnCache.hasCached()) {
evaluateWithSelection(selection);
}
}
// here (in FilterAndProjectOperator: filterColumn / positionCount)
boolean[] selection = new boolean[deviceSchemaBatch.size()];
for (int i = 0; i < selection.length; i++) {
selection[i] = satisfy(curFilterColumn, i);
}
for (ColumnTransformer transformer : commonTransformerList) {
transformer.tryEvaluateWithSelection(selection);
filterResultColumns.add(transformer.getColumn());
}With this change, both UPDATEs and the SELECT return the expected results in my local run, and the calc-commons tests plus the datanode *ColumnTransformerTest / DevicePredicateHandlerTest still pass.
This block also duplicates FilterAndProjectOperator.getFilterTsBlock. The two copies have already drifted once: base had tryEvaluate() there but not here, which is the NPE this PR fixes. A single helper next to constructFilteredTsBlock would keep the fix in one place.
| // CASE evaluation can clear a common expression after the filter used it. Recompute it | ||
| // before materializing the column for projection within this same input block. | ||
| columnTransformer.tryEvaluate(); | ||
| resultColumns.add(columnTransformer.getColumn()); |
There was a problem hiding this comment.
Recomputing here also re-runs stateful functions on the same block after CASE has cleared them. Before this PR the multi-block case below failed; now it silently returns wrong values:
CREATE TABLE dt2 (time TIMESTAMP TIME, dev STRING TAG, x DOUBLE FIELD);
INSERT INTO dt2(time, dev, x) VALUES (1, 'd0', 1.0), (2, 'd0', 3.0), (3, 'd0', 6.0);
FLUSH;
INSERT INTO dt2(time, dev, x) VALUES (4, 'd0', 10.0), (5, 'd0', 11.0);
FLUSH;
SELECT time, x, diff(x) AS d FROM dt2 WHERE CASE WHEN diff(x) > 1.0 THEN 1.0 ELSE 0.0 END > 0.0;(1C1D with max_tsblock_line_number=1, so [1, 3, 6] and [10, 11] reach this operator as separate blocks)
- expected:
d = 2.0, 3.0, 4.0at time 2, 3, 4 - base:
301: Index 2 out of bounds for length 2 - this PR: time 4 returns
d = -1.0. The first WHEN already computeddifffor[10, 11]and advanced its state to 11. CASE then clears that column, and this line runsdiffagain on the same block (10 - 11). - With
ELSE 1.0(every row passes), time 1 returns-5.0instead of NULL.
A selection mask (see the DeviceAttributeUpdater comment) doesn't help here, because DIFF evaluates every row anyway. The column computed by the first WHEN was complete and valid; it is lost only because CASE clears its WHEN subtree (see the root-cause note in the review summary).
| protected boolean shouldClearChildrenCache() { | ||
| // Clearing a cached CASE during evaluation must preserve pending sibling columns. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This preserves a pending sibling column only when the shared node is itself a CASE. CASE's own cleanup (L222-L227) still clears its WHEN/THEN/ELSE subtrees recursively. Any non-CASE node shared with an operand that the parent has already computed but not read yet is therefore wiped within the same block. All of these fail the same way on base and on this PR, with a single input block:
| SQL | expected | actual |
|---|---|---|
SELECT s1 + CASE WHEN s1 > 0 THEN 2 ELSE 3 END FROM p (s1 = 1, -2) |
3.0, 1.0 | NULL, NULL (tree model too) |
SELECT time, s1 FROM p WHERE s1 + CASE WHEN s1 > 0 THEN 2 ELSE 3 END > 0 |
2 rows | empty set |
SELECT s1 > CASE WHEN s1 > 0 THEN 2 ELSE -3 END FROM p |
false, true | 301: … "leftColumn" is null |
SHOW DEVICES / COUNT DEVICES … WHERE src = CASE WHEN src = 'match' THEN 'match' ELSE 'other' END |
all devices | 301: … "leftColumn" is null |
UPDATE t SET a = concat(src, CASE WHEN src = 'match' THEN '-m' ELSE '-o' END) |
match-m / other-o |
succeeds, but every a becomes NULL (old value lost) |
greatest(s1, CASE …), s1 BETWEEN -5 AND CASE …, substring(s, 1, CASE …), CASE WHEN s IS NOT NULL THEN substring(s, 1, length(s) - 1) END |
values | NULL |
Cause: BinaryColumnTransformer.evaluate(), TernaryColumnTransformer.evaluate() / evaluateWithSelection(), MultiColumnTransformer.evaluate() and MappableUDFColumnTransformer.evaluate() / evaluateWithSelection() evaluate all children first and call getColumn() afterwards. When a later child is (or contains) a CASE, its cleanup clears the shared node, and the earlier child's read returns null / 0 positions.
BinaryColumnTransformer.evaluateWithSelection() already avoids this by reading the left column before evaluating the right one (see its comment). I applied that rule everywhere: read each child's column and position count right after evaluating it, before evaluating the next sibling. In my local run that fixes every row above, and the calc-commons tests and datanode transformer tests still pass.
The same argument applies to this opt-out itself: "preserve pending sibling columns" holds for every node type, not just CASE. The new tests only use the CASE … = src orientation, which happens to avoid the problem (see the IT comment).
| protected void forEachChild(Consumer<ColumnTransformer> action) { | ||
| columnTransformerList.forEach(action); | ||
| } |
There was a problem hiding this comment.
Not changed by this PR, but same mechanism, same class: evaluateWithSelection() reads getColumnCachePositionCount() at L72 after getColumn() has already released the first child's column (when that child has a single reference), so the result has 0 rows. Any multi-argument function under THEN/ELSE whose first argument is referenced only once returns NULL, on base and on this PR:
SELECT time, CASE WHEN time > 0 THEN coalesce(s1, 0.0) END FROM p -- expected 1.0, -2.0; actual NULL, NULL
SELECT time, CASE WHEN time > 0 THEN greatest(s1, 0.5) END FROM p -- expected 1.0, 0.5; actual NULL, NULL8bf2294 fixed this exact pattern in LogicAnd / LogicOr / LogicalAndMulti / LogicalOrMulti / MappableUDF but missed this class. Reading the position count before getColumn(), as evaluate() does at L50-L51, fixes it.
| protected void forEachChild(Consumer<ColumnTransformer> action) { | ||
| for (Pair<ColumnTransformer, ColumnTransformer> whenThenColumnTransformer : | ||
| whenThenTransformers) { | ||
| action.accept(whenThenColumnTransformer.left); | ||
| action.accept(whenThenColumnTransformer.right); | ||
| } | ||
| action.accept(elseTransformer); | ||
| } |
There was a problem hiding this comment.
Nit: CASE now keeps the same child list in three places: here, the clear loop in doTransform() (L222-L227) and close() (L255-L263). Since the goal is a single child-edge definition, consider forEachChild(ColumnTransformer::clearCache) in doTransform() and forEachChild(ColumnTransformer::close) in close(). The same applies to the close() methods of Unary/Binary/Ternary/Multi/MappableUDF. That way a new child edge cannot be added to one list and missed in another.
| for (ColumnTransformer columnTransformer : filterCacheTransformers) { | ||
| columnTransformer.clearCacheForNewInput(); | ||
| } |
There was a problem hiding this comment.
Optional (same as base, not blocking): the reset runs when the next block arrives. Columns left over from the previous block therefore stay reachable while the driver is blocked and after the last block; an example is a shared expression recomputed after CASE, which keeps an unconsumed reference. Resetting at the end of next() (in a finally) would release them as soon as the output block is built.
| statement.execute( | ||
| "UPDATE common_cache SET a = src " | ||
| + "WHERE CASE WHEN src = 'match' THEN true ELSE false END"); |
There was a problem hiding this comment.
This test does not exercise the common-expression path it is named after. src is a bare column reference: ColumnTransformerBuilder.visitSymbolReference turns it into a leaf without consulting hasSeen, so commonTransformerList stays empty. The same UPDATE already returns the correct result on base (checked with max_tsblock_line_number=1 and one schema region). The unit test at DevicePredicateHandlerTest L167 likewise uses a leaf as the common transformer, which the planner never produces.
A statement that does go through the common-expression path:
UPDATE common_cache SET a = upper(src) WHERE CASE WHEN upper(src) = 'MATCH' THEN true ELSE false END- base:
305: internal error. statemachine throws a runtime exception: java.lang.NullPointerException: Cannot invoke "org.apache.tsfile.block.column.Column.isNull(int)" because "curColumn" is null - this PR: d0/d1 get
MATCH, d2/d3 stay NULL
Please also cover the guarded-cast UPDATE from the DeviceAttributeUpdater comment.
| try (Connection connection = EnvFactory.getEnv().getConnection(BaseEnv.TABLE_SQL_DIALECT); | ||
| Statement statement = connection.createStatement()) { | ||
| createTable(statement, "predicate_cache"); | ||
| String predicate = CASE_RESULT + " = src"; |
There was a problem hiding this comment.
All new schema tests (and DevicePredicateHandlerTest) put the CASE on the left: CASE … = src. The mirrored predicate src = CASE … END already fails in the first batch on both base and this PR (301: … "leftColumn" is null; see the comment on shouldClearChildrenCache()). Please cover both orientations once that is fixed.
| } | ||
| } | ||
|
|
||
| private static class TsBlockSourceOperator implements Operator { |
There was a problem hiding this comment.
Nit: this duplicates the production ValuesOperator in the same package. new ValuesOperator(new TestOperatorContext(), Arrays.asList(blocks)) should do the same job, and it won't need updating when the Operator interface changes.
| @Test | ||
| public void testAttributeUpdaterRecomputesCommonExpressionClearedByCase() throws Exception { | ||
| Graph filter = new Graph(); | ||
| ColumnTransformer shared = filter.input(0); |
There was a problem hiding this comment.
filter.input(0) as a common transformer cannot come from the planner (bare column references never become common expressions). This test therefore exercises the recompute mechanism, but not a DAG the planner builds. A shared non-leaf node (for example, a function over input) would match what UPDATE … SET a = upper(src) WHERE CASE WHEN upper(src) = 'MATCH' … produces.
Share masked common-column materialization between query and schema UPDATE paths. Retain sibling columns before later evaluations can clear their caches, and read multi-argument sizes from retained columns.
Track the known DIFF failure with a correct-result reproducer. Guard common-expression evaluation counts, document helper side effects and shallow array accounting, and retain constructor descriptors used by precompiled subclasses.
Description
Closes #18792.
CASE cleanup can invalidate a shared aggregate or intermediate expression. A later consumer recomputes it with its full reference count, leaving an unconsumed cached column that survives into the next TsBlock. Reusing the DAG can then return stale values, filter rows incorrectly, or read a column with the wrong size.
Invalidate every node before initializing a new input in
FilterAndProjectOperator,DevicePredicateHandler, andDeviceAttributeUpdater. Each evaluation phase collects an identity-deduplicated flat array once during construction, including CASE descendants. The filter array includes filter/common roots; the projection array includes all output roots. Block boundaries only loop over the array and clear each node's own cache. The copy constructor reuses these arrays, andramBytesUsed()accounts for them.Child edges are declared once for collection and evaluation-time cleanup.
clearCacheForNewInput()is final and clears only the current node. Evaluation-timeclearCache()preserves the existing CASE boundary and stays non-final for Mockito compatibility.Common expressions used by filtering and projection are materialized through one helper shared by query operators and attribute updates. If CASE invalidated a common cache,
tryEvaluateWithSelection()recomputes it using the rows that passed WHERE. This fixes the earlier revision's guarded-cast UPDATE regression: rejectedsrc = 'n/a'rows must not be converted to INT32, including when no device matches. An empty common-expression list allocates no selection mask.Retain each child's column immediately after evaluating it in Binary, Ternary, Multi, and Mappable UDF transformers, before a later sibling can clear shared caches. Multi transformers also read the position count from the retained first column instead of its released cache. These fixes are necessary for safe selection-mode recomputation: a common
coalesceexpression passed at5e0cb1c8a9but returned NULL with the mask alone.ColumnCachereference-count semantics are unchanged.Regression coverage
upper(src)expression.IoTDBCaseWhenThenTableITcover the original aggregation reproducer, the pending-sibling guard, guarded common casts, and CASE operands/multi-argument functions. The pending-sibling guard passes on the original code; it protects against making evaluation-time CASE cleanup recursively cross CASE boundaries.IoTDBCaseDeviceCacheITcover SHOW/COUNT DEVICES with both predicate orientations, cross-batch projection,concat(src, CASE ...)updates, commonupper(src), and guarded casts with zero/one matching device. The common-expression tests use a function rather than a bare symbol because the planner does not extract bare symbols as common expressions. DELETE DEVICES rejects CASE predicates during analysis, so blacklist construction is covered by its unit test.Follow-up scope
Repeated stateful
diff()evaluation is still unresolved and tracked in #18863. Neither the filter mask nor retaining sibling columns prevents CASE from invalidating a stateful expression that is later recomputed. Selection-local results versus reusable full-column cache ownership will be designed separately. A deterministic operator regression asserts the correct[2, 3]then[4]result, is explicitly@Ignorepending #18863, and was invoked directly to confirm the currentexpected:<4.0> but was:<-1.0>failure. It does not assert the wrong value as correct behavior.The operator-owned public static helper will be decoupled separately in #18862. Its Javadoc now explicitly documents evaluation, descendant cache mutation, reference-count consumption, and row alignment. The pre-existing tree-model AND/CASE wrong-column bug is tracked in #18864 and is outside this PR.
The pending sibling-column finding and multi-argument position-count finding are addressed by the column-retention changes above.
Validation
Cannot cast n/a to INT32 type. Five additional operator tests failed before the child-column fixes and now pass. A class-isolation check confirms the updatedupper(src)schema test fails with the original schema classes (null common column) and passes with the fixed classes.d7239384fd, all eight selected integration tests passed on TableSimpleIT (1 ConfigNode + 1 DataNode), with the dependency reactor rebuilt. This follow-up reran the exact common-expression SQL test in the base/head comparison below; the other seven ITs were not rerun for the added compatibility overloads, documentation, and unit tests.IoTDBCaseDeviceCacheIT#testAttributeCommonExpressionClearedByCaseSQL test was compared in 1C1D with identical dependencies: all 15 PR-modified production source files were compiled from merge basefbd5fd22a6and substituted in the test-node jars, then the original HEAD jars were restored with SHA-256 verification. The base-source run failed with a 305 null-column error; HEADd7239384fdpassed. This is a source-isolated comparison of all changed classes, not a claim of a clean full-base reactor build.DevicePredicateHandlerandDeviceUpdaterare retained. Subclasses precompiled against the base signatures failed withNoSuchMethodErrorbefore the overloads and link successfully with them. The array memory comment clarifies that shallow accounting includes headers and reference slots, but does not account for transformer objects.git diff --checkpassed.Local IT verification used
-Drat.skip=truebecause an unrelated pre-existing, untracked SQL file fails the license scan. Added test files have the ASF header.Performance check for construction-time collection (
5e0cb1c8a9)JMH 1.37 compared
50c38c16ddwith5e0cb1c8a9using the actual operator, one-row blocks, eight rotating prebuilt blocks, and correctness checks. Both used the same dependencies, Corretto 17.0.13/aarch64, 512 MiB G1, two forks per revision/scenario with reversed order in round two, 3 × 1 s warmup, and 4 × 1 s measurement.x + 1S = x + 1(x0 + 1) > 0, project 16 columnsAllocation reductions were consistent. Timing varied across forks, with overlapping ranges in the filtered case. These compare two PR revisions, not end-to-end SQL or the unfixed merge base. This check predates the filter-mask/column-retention changes and has not been rerun for them. Steady-state invalidation is an
O(V)flat loop; collection isO(V + E)once during construction.Common-expression materialization cost
The JMH scenarios above have no extracted filter/projection common expression; they do not measure the mask work added by common-column materialization. For every block with a nonempty common list, the helper allocates one
boolean[n]and scans the originalninput rows once, even if all common columns remain cached. On the measured JVM, the mask array alone is 24 B for one row, 1040 B for 1024 rows, and 10016 B for 10000 rows. These are allocation-size measurements, not operator timing or SQL latency estimates.The planner replaces a projection-side extracted common expression with an
IdentityColumnTransformerreading the materialized filtered column. Clearing the projection DAG does not clear or re-execute the filter-side common transformer. The new counting test verifies:An expression computed inside a selected THEN/ELSE branch can instead be evaluated with two different masks: the branch mask followed by the final WHERE mask during materialization. That repeated work is real and can matter for expensive expressions; the selection-cache lifecycle follow-up #18863 must address it. The projection phase still reads an Identity input rather than evaluating the expensive expression again.
This PR has:
Key changed/added classes
FilterAndProjectOperatorColumnTransformerand its composite subclassesAbstractCaseWhenThenColumnTransformerDevicePredicateHandlerandDeviceAttributeUpdaterFilterAndProjectOperatorTestandDevicePredicateHandlerTestIoTDBCaseWhenThenTableITandIoTDBCaseDeviceCacheIT